-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove sticky posts setting when we inherit the query #40656
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes it for me in the front end. We still need to fix the editor as well, but that can happen in a followup.
I am not able to reproduce the bug, I don't want to be rude but it would help me if the instructions were more detailed.
What settings exactly does the query loop need to have for me to reproduce the issue?
I am assuming this means the option labeled with "Inherit query from template" |
For me the reproduction steps are these:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @draganescu ! Sticky posts have many nuances in the Query Loop context.
For example your comment here:
Another problem here is that when Inherit query from template is set the sticky preferences should either be hidden or they should be merged in the global query. Which one is more expected for the user?
I was thinking a while ago to hide them, but it seems preferable if we find a proper solution for this.
Another 'sticky' problem we have is that they're shown in every page results in a custom Query Loop block that has include sticky
.
Screen.Recording.2022-04-29.at.12.19.58.PM.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to make the review more prominent not to merge right now because of the approval and the breakage.
c8cdd79
to
9f565df
Compare
@ntsekouras in 066437c I added a change that unsets all query block defaults which are not editable by the user when inherit is
|
066437c
to
7d556af
Compare
Haven't yet looked into the TLDR:
|
I'm not sure yet about this and how to approach it. Right now I'm thinking that it should be better to just hide the For the specific use case now ( A solution could be specific to sticky behavior
What do you think? |
I think sticky posts are important for certain workflows. I would proceed from the feedback gathered here, plus a chat with @jameskoster, plus the fact that we don't really have straight answers to:
|
Size Change: +2.46 kB (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Will this functionality be ported to 6.0.1 or 6.0.2 Seems like a big enough bug that worth including. |
I did something and I see some changes that should not be here. Will fix. @spacedmonkey I don't see why not. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for iterating on this one @draganescu !
In general after testing this seems to work well. We would need to also make similar changes to No Results block.
A small concern would be this removed snippet:
if ( empty( $query_args['post_type'] ) && is_singular() ) {
$query_args['post_type'] = get_post_type( get_the_ID() );
}
But I don't think it breaks anything. My understanding is that this would just have some fallback in the case of inherit
in a single page, where now it shows nothing. This is not a blocker for me as in general it doesn't make sense to have such a query in a single page. I think this is part of the context detection issue we have, where we should better handle the Query Loop based on the context it lives. --cc @aristath
Finally, I don't think we should rush it in core, as it's not a bug that creates problems. To the contrary, it would be better to give it time to test it in the plugin for any regression/issues.
If a page/post template does not exist, then the WP template system and hierarchies fallback to using the main |
Thanks for explaining Ari!
I just tested that(only |
@ntsekouras said:
And so I won't be rushing this into 6.0.1 RC1 that happens in a few hours. Instead, I added the backport label to flag this for 6.0.2 release squad. |
@ntsekouras I'll add the updates to the no results block. @huubi and @spacedmonkey I've added you both as co-authors since the optimisation to the query was not part of the original intention or code of the PR. |
@ntsekouras while adding the update to the no results block I realized I don't understand why do we need to query anything if we render a no results page, since we know there are no results? What was the intention there? Later edit: It seems this block could get the information if there are no posts from context rather than re-querying just to decide if it is going to render itself. |
Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Huub <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @draganescu and also @huubi and @spacedmonkey for contributing in this one!
I'd appreciate some more testing about this #40656 (comment), but other than that looks good. With 🟢 CI let's ship.
CS alignments fix
* reset the sticky posts query var for inherited query * merge query vars into query defaults and unset empty s query var * unser all query block defaults which are not editable when inherit is true * fixes linting PHP problems * uses global query as is and hides and UI that overrides it * removes mistaken changes * removes query overriding on inherit for no results block, although why? * Update packages/block-library/src/query-no-results/index.php * Adds co-authors Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Huub <[email protected]> * Update packages/block-library/src/query-no-results/index.php CS alignments fix Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Huub <[email protected]> Co-authored-by: Ari Stathopoulos <[email protected]>
Cherry-picked with 493d14f for WordPress 6.0.2 release. |
What?
Fixes #38979
Why?
Because the setting not just doesn't work.
How?
We hide the setting when inherit is true.
Testing Instructions
Notes
This PR has explored allowing sticky post behaviour on inherit but the result was consistently unreliable, one or another query var would be overwritten either by WP_Query or by the query block.
Therefore the PR hides the setting and fixes the server rendering of the block to truly inherit, without interfering with the current WP_Query in any way.
Co-authored-by: Jonny Harris [email protected]
Co-authored-by: Huub [email protected]